feat(ios): support explicit iOS simulator keychain reset - #2345
Conversation
`settings clear-app-state` never touched keychain-backed credentials (e.g. Firebase auth), so a customer's fresh-install reset via the CLI left an app signed in when their in-app reset button did not (#2282). simctl exposes no per-app keychain reset, only a whole-simulator one (`simctl keychain <device> reset`), so this ships as a separate, explicit `settings reset-keychain clear` command rather than folding it into `clear-app-state` — callers opt in knowing the scope is the whole simulator, not just the app under test. Split the pre-existing `apps.test.ts` and `snapshot-handler.test.ts` suites along the `app-settings.ts`/`snapshot-settings.ts` modules they actually mirror, since both were already over the test-file-size tripwire and could not grow further.
|
Size Report
Startup median (7 runs, lower is better):
|
|
Please reject app-scoped input before resetting anything. The CLI accepts settings reset-keychain clear com.example.app and drops the extra argument; the direct daemon parser also accepts it. That can turn an apparent per-app reset into a whole-Simulator keychain wipe. Require exactly reset-keychain clear, and add CLI and direct-daemon tests proving an extra app argument causes no settings mutation. The remaining validation from #2282 is also needed: authenticate with keychain-backed credentials, reset, relaunch, and verify the app starts signed out. The reported disposable-Simulator command success proves invocation, not that login outcome. Reviewed at 3237128. |
…eychain fixture settings reset-keychain clear <extra-arg> silently dropped the extra argument in both the CLI reader and the direct-daemon parser, so a caller expecting per-app scoping could get a whole-simulator wipe without any signal something was off. Reject it instead in both places, with tests proving no settings mutation happens. Also add a small keychain-backed "auth" fixture to the test-app's automation lab (expo-secure-store) so the settings reset-keychain guarantee has a real regression surface: authenticate, verify the credential survives clear-app-state and a plain relaunch, then verify reset-keychain actually clears it. Validated live against a disposable iOS simulator.
|
Addressed both points in 98ff05d: Extra positional argument — fixed in both places: the CLI reader ( Real login-outcome validation — added a small "Keychain-backed auth" section to
Disposable simulator deleted afterward. Full unit suite (9063 tests), typecheck, and lint all green on the updated branch. |
|
The head is unchanged at 3237128, so the extra-argument validation finding and signed-out-state evidence request still stand. Android Smoke has also finished red at the post-alert canary (wait for Alert result: cancelled). This is the same failure seen on other PRs; it does not change the keychain review, but the required lane still needs to pass. |
|
The new head 98ff05d addresses both review points. Extra arguments now fail before mutation on the CLI and direct-daemon paths, with regressions for both. The reported SecureStore check covers persistence through relaunch and clear-app-state, then signed-out state after keychain reset. Ready for human review. The previous comment referred to the superseded head; current CI is still running, with no failed checks at this review. |
thymikee
added
ready-for-human
|
Update: Android Release has since failed at 98ff05d (Gradle reports its Node subprocess exited 1). I removed ready-for-human while that new failure is investigated. The code review remains clean; please establish the build failure cause and get the release build passing. |
|
The iOS smoke lane also finished red: the depth-1 bridge assertion received XCTest fallback after target-resolution-failed. Keep this separate from the keychain behavior and Android release-build failure; establish the cause and rerun the affected lane before merge. |
1 participant
Summary
settings clear-app-stateclears the app's data container but never touched Keychain, so a customer's fresh-install reset left the app signed in via Firebase auth even though their in-app "Reset to Fresh Install" button cleared it.settings reset-keychain clear, a new iOS-simulator-only settings action that runsxcrun simctl keychain <device> reset.clear-app-state: simctl has no per-app keychain reset, only a whole-simulator one, so folding it into the existing per-app command would silently widen its blast radius.reset-keychaindocuments and requires an explicit opt-in to that wider scope instead.settingsalready dispatches generically to the platform'ssetSettingoperation, so no daemon wire changes were needed beyond the shared contracts/usage-string updates.website/docs/docs/commands.mdand in the CLI's own--helptext, including the "pair withclear-app-statefor a full fresh-install reset" guidance the issue asked for.apps.test.tsandsnapshot-handler.test.tssuites along theapp-settings.ts/snapshot-settings.tsmodules they actually test — both were already over the repo's test-file-size tripwire and could not grow to hold the new tests.Test plan
pnpm typecheckpnpm lintpnpm test:unit(full suite, 9061 passed)agent-device settings reset-keychain clear --udid <udid>against it — succeeded and returned the expected{ scope: 'simulator', cleared: true }payload; confirmed an invalid state (nope) is rejected before touching the device. Disposable simulator deleted afterward.